this feels like it should be a responsibility of the caller - otherwise, we should change the send function tot take a list of message instead of a full RPCMsg - also, a single message might also be bigger than the limit so the utility of this change seems limited compared to the complexity it brings.

I agree. I only did it here to use the encoded message and not add the overhead of calculating the message size.

If individual messages bigger than the size limit are a common case, we should revisit the message limit. This change allows us to send messages below the size limit that can be sent individually but not as a group.

Now we can split the RPCMsg before calling send. But then we need to create a seq to pass it. Wdyt @arnetheduck ?

Most of the time, sendEncoded is used directly here:
https://github.com/status-im/nim-libp2p/blob/c6aa085e98e7526cb8d4415cb9a7f886e6dcab30/libp2p/protocols/pubsub/pubsub.nim#L199-L202

So this fix won't be enough

I believe the code related to an iwant response uses send https://github.com/status-im/nim-libp2p/blob/0cdfb0fedcb5f19199c3f3607dd6ff67740fb937/libp2p/protocols/pubsub/gossipsub.nim#L298-L300.

But I can also move this solution to sendEncoded, as it is called by send. Wdyt?

Good point, no preference here

Btw in the current implementation, I split the msgs after p.sendObservers(mm) and sendMetrics(mm). Do those procs require splitting too?

I would say no

But to do that we need to create an RPCMsg, add Messages, and calculate its size.

the size can be pre-computed - this would be the other way to write the above loop (this is what most protobuf encoders do btw - they precompute size then allocate memory in a single shot).

the overhead of rpcmsg itself is static so it can easily be accounted for - this would be the other way to implement this, ie precompute sizes and split based on them and the rpcmsg overhead itself.

Efficiency is indeed key in gossipsub because of its multiplicative nature - any overhead quickly explodes as message counts go up - at this point, we should be mindful of it and indeed, one of the best things we could do in terms of overall efficiency of gossipsub (and therefore nimbus) would be to improve the efficiency of rpcmsg encoding and decoding in general.

so how about this?

1 - We create a len proc for RPCMsg and all other types used within it that don't have one already.
2 - We use those procs to estimate the serialized size of a RPCMsg and split it based on that. Let's say len(serializedRPCMsg) = 1.1 * len(rpcMsg) - guessing a 10% Protobuf overhead.
3 - We send each RPCMsg individually.

A pseud-code below:

Function splitRPCMsg(rpcMsg: RPCMsg, maxSize: int) -> List[RPCMsg]:
    Initialize newRPCMsgs as an empty list of RPCMsg objects
    Initialize currentRPCMsg as a new RPCMsg object with the same non-message fields. We sent them only in the first RPCMsg, the next ones have only Messages.
    Initialize currentSize = len(currentRPCMsg)  # This includes the size of control and other fields

    For each msg in rpcMsg.messages:
        msgSize = len(msg)

        # Check if adding the next message will exceed maxSize
        If (currentSize + msgSize) * 1.1 > maxSize:
            Add currentRPCMsg to newRPCMsgs
            Initialize currentRPCMsg as a new, empty RPCMsg object
            currentSize = 0

        # Add the message to the current RPCMsg
        Add msg to currentRPCMsg.messages
        currentSize += msgSize

    # Check if there is a non-empty currentRPCMsg left to be added
    If currentSize > 0:
        Add currentRPCMsg to newRPCMsgs

    Return newRPCMsgs

this can be done deterministically, there's code in nim-protobuf-serialization showing how - also, unless there's need for it, better to return the encoded messages (or even better, turn this into an iterator which yields the encoded messages so that there is no complex seq involved

Honestly we don't have to be super precise, even if we just split once we reach limit / 2 without counting pb overhead, each message would still be >500kb with probably <50 bytes overhead (or 0.01% overhead, that's assuming 1mb max message size)
That's why I also proposed originally to split in the handleIWant directly (or somewhere like that). Even just sending each reply in a separate message wouldn't have crazy overhead

We don't use nim-protobuf-serialization in libp2p. Tried importing it and using computeObjectSize, but got errors about lack of pragma annotations and so on.

..

Done

this copy needs to be avoided when no splitting is needed

I believe when no split is needed the iterator isn't called:

  if encoded.len > p.maxMessageSize:
    for encodedSplitMsg in splitRPCMsg(p, msg, p.maxMessageSize, anonymize):
      asyncSpawn p.sendEncoded(encodedSplitMsg)
  else:
    # If the message size is within limits, send it as is
    debug "sending msg to peer", peer = p, rpcMsg = shortLog(msg)
    asyncSpawn p.sendEncoded(encoded)

Why this?
If you remove it, you can also make onNewPeer private again

This already existed, I just moved it to this file.

I did it cause I tried to use getPubSubPeer in my test, but it isn't necessary anymore. I tried to revert it, but I got multiple errors as testgossipinternal calls onNewPeer and it works only because gossipsub file was included there. I can no longer do that as I import utils in testgossipinternal now and it causes an error. Yeah, I know, messy.

This code (& the len part) seems very prone to failure, if we add a field to RPCMsg, update a field, etc
We should at the very least add a comment to the RPCMsg declaration to warn about that, and ideally find a better solution

It seems that the best solution would be to use nim-protobuf-ser, but we can't yet unfortunately

As an alternative, wouldn't it be simpler to modify encodeRpcMsg? It could take a maxSize, and return a seq[seq[byte]], or even just concatenate the PBs directly internally (would require some tweaks to the pubsubpeer)

I think adding the comment is a good and easy-to-do improvement. But not sure about splitting in encodeRpcMsg, it seems too much abstraction leakage.

Added the comment